feat: migrate video shorts to learning resources API#3361
feat: migrate video shorts to learning resources API#3361daniellefrappier18 wants to merge 8 commits into
Conversation
OpenAPI ChangesNo changes detected Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
Pull request overview
Migrates the Home Page “Video Shorts” feature from the legacy v0 VideoShort API shape to the v1 Learning Resources VideoResource shape, including switching playback to video.streaming_url.
Changes:
- Replace v0
useVideoShortsListusage in the Home Page section with a v1 Learning Resources search-based hook. - Update the Video Shorts modal to consume
VideoResourceand playvideo.video.streaming_urlwith revised portrait sizing. - Update modal tests to use v1 factories/types and validate
streaming_urlhandling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontends/main/src/app-pages/HomePage/VideoShortsSection.tsx | Swaps data source to v1 VideoResource list and updates thumbnail rendering + keys. |
| frontends/main/src/app-pages/HomePage/VideoShortsModal.tsx | Switches modal playback to streaming_url and adjusts slide sizing/styling. |
| frontends/main/src/app-pages/HomePage/VideoShortsModal.test.tsx | Updates tests to v1 VideoResource factories and adjusts assertions for new fields. |
| frontends/api/src/hooks/videoShorts/index.ts | Adds a new hook that fetches Video Shorts via Learning Resources search and filters to videos. |
Comments suppressed due to low confidence (1)
frontends/main/src/app-pages/HomePage/VideoShortsModal.tsx:189
video.video.streaming_urlis documented/produced as an HLS.m3u8URL (see backend help_text/tests). A plain<video src="...m3u8">will not play on many browsers (notably Chrome/Firefox) without an HLS-capable player (e.g., video.js/hls.js) or a non-HLS fallback. Consider reusing the existingVideoJsPlayer/VideoResourcePlayer+resolveVideoSourcespattern used elsewhere inmainso HLS works cross-browser, rather than relying on native<video>.
<Video
ref={refCallback}
onClick={onVideoClick}
src={src}
autoPlay
muted
playsInline
webkit-playsinline="true"
controlsList="nofullscreen"
disablePictureInPicture
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
frontends/main/src/app-pages/HomePage/VideoShortsModal.tsx:196
video.video?.streaming_urlwill often be an HLS.m3u8(per existing VideoResource playback paths). Rendering it directly in a native<video src=...>will fail to play on browsers without native HLS support (e.g., Chrome/Firefox). Consider reusing the existingVideoJsPlayer/VideoResourcePlayer+resolveVideoSourcesapproach (or an HLS polyfill) so Video Shorts are playable cross-browser.
<Video
ref={refCallback}
onClick={onVideoClick}
src={src}
autoPlay
muted
playsInline
webkit-playsinline="true"
controlsList="nofullscreen"
disablePictureInPicture
width={videoWidth}
height={videoHeight}
preload="metadata"
loop
/>
|
For a solution to the videos appearing "soft" for a brief moment on startup due to adaptive HLS rendition selection @mbertrand has created the following issue. Matt implemented ^ and this has solved the issue. |
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Looks good.. One request!
I did not notice any degradation of quality in first few moments. Maybe that's @mbertrand 's recent change.
|
|
||
| const VideoShortsSection = () => { | ||
| const { data, isLoading } = useVideoShortsList() | ||
| const { data, isLoading } = useVideoShortsLearningResources() |
There was a problem hiding this comment.
This comment is long-winded, it boils down to a few things:
- use the existing
learningResourceQueries.searchquery- After this, I expect you could remove the whole
api/hooks/video_shortsdirectory
- After this, I expect you could remove the whole
- I think we should add
sortby: "new"- The default search is pretty similar when there's no
qfilter (search query text), but IMO best to explicitly usenew, that's closest to old-published_at, I think.
- The default search is pretty similar when there's no
Rather than adding a new hook useVideoShortsLearningResources, I'd suggest re-using the existing learningResourceQueries.search query. Something like below.
Also:
- filter on
resource_typeat the API level rather than afterwards. (It probably doesn't matter much since you were filtering on "Video Short" category anyway)- Generally filtering serverside is better...if you request 50 results and filter client-side, you might end up with fewer. But, as I said, it shouldn't matter here. Still good to do.
I'd suggest using
const { data, isLoading } = useQuery({
...learningResourceQueries.search({
resource_category: ["Video Short"],
resource_type: [ResourceTypeEnum.Video],
limit: 50,
sortBy: "new"
}),
select: (data) =>
data?.result.results.filter(
(r) => r.resource_type === ResourceTypeEnum.Video,
),
})Two notes about the select i suggested:
- This query returns
{ results, time }after Search response time indicator for admin users #3186 ... it's the only query that does this, for the reasons mentioned in that pr. - The filter inside
selectis redundant. I kept it just to make TS happy.
Also
- generally nowadays we prefer
useQuery(queryOptions)rather than defining custom query hooks. - In older versions of
@tanstack/react-query, defining custom query hooks was the best way, but the queryOptions helper they added in v5 eliminated the need for custom query hooks. (Custom mutation hooks still big helpful.)
There was a problem hiding this comment.
Thanks! I followed the pattern from the existing useVideoShortsList hook when writing useVideoShortsLearningResources, but I can see now that using useQuery(queryOptions) is the cleaner approach. I'll keep that in mind going forward.
…move videoShorts hooks
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/11036
Description (What does it do?)
This branch migrates Video Shorts from the old v0 API shape to the v1 learning-resources API shape. The modal now uses VideoResource and plays video.streaming_url instead of the old direct MP4 URL.
Screenshots (if appropriate):
new-video-shorts.mov
How can this be tested?
Automated tests
Manual testing
Additional Context